Added quota contoll without the service control client library#92
Added quota contoll without the service control client library#92mangchiandjjoe wants to merge 13 commits intoistio:rate_limitingfrom mangchiandjjoe:master
Conversation
* Send all attributes. * Remove unused const strings. * Address comment.
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
Running presubmits at https://istio-testing.appspot.com/job/proxy/job/presubmit/56/ ... |
|
proxy/presubmit failed. Details at https://istio-testing.appspot.com/job/proxy/job/presubmit/56/. |
|
Running presubmits at https://istio-testing.appspot.com/job/proxy/job/presubmit/57/ ... |
|
proxy/presubmit failed. Details at https://istio-testing.appspot.com/job/proxy/job/presubmit/57/. |
|
Running presubmits at https://istio-testing.appspot.com/job/proxy/job/presubmit/58/ ... |
|
proxy/presubmit failed. Details at https://istio-testing.appspot.com/job/proxy/job/presubmit/58/. |
|
Please run script/check-style, this is why Jenkins is failing: script/check-style: line 26: /home/jenkins/clang/bin/clang-format: No such file or directory |
|
Thanks. I just ran the tool. |
|
Running presubmits at https://istio-testing.appspot.com/job/proxy/job/presubmit/59/ ... |
|
proxy/presubmit failed. Details at https://istio-testing.appspot.com/job/proxy/job/presubmit/59/. |
| @@ -37,7 +37,8 @@ typedef std::shared_ptr<HttpRequestData> HttpRequestDataPtr; | |||
| class HttpControl final : public Logger::Loggable<Logger::Id::http> { | |||
| public: | |||
| // The constructor. | |||
There was a problem hiding this comment.
these two changes should not be here
- you need to base your change to istio/proxy:rate_limting branch. It seems that you are based on master branch.
- or you need to sync rate_limiting branch to master
There was a problem hiding this comment.
ok I will do that.
let me close this PR and I will create a new one.
Thanks.
There was a problem hiding this comment.
I created a new PR after I re-base the repo
Please review the new one #93
| return; | ||
| } | ||
|
|
||
| if (context->api_key().empty()) { |
There was a problem hiding this comment.
we need to call quota for all requests.
There was a problem hiding this comment.
Removed the paragraph
|
|
||
| sa_token_quota_ = new auth::ServiceAccountToken(env); | ||
| sa_token_quota_->SetClientAuthSecret( | ||
| server_config->google_authentication_secret()); |
There was a problem hiding this comment.
You should NOT get token from server_file. You could define a new type
JWT_TOKEN_FOR_QUOTA_CONTROL, and set it audience. then you can get its token.
| // Generate QuotaAggregationOptions | ||
| QuotaAggregationOptions GetQuotaAggregationOptions( | ||
| const ServerConfig* server_config, | ||
| const ::google::api::Service* service_config) { |
There was a problem hiding this comment.
I don't see service_config is used
There was a problem hiding this comment.
Removed the argument
| delete response; | ||
| }; | ||
|
|
||
| AllocateQuotaRequest* quota_request_copy = new AllocateQuotaRequest(*request); |
| // TODO(jaebong) Temporarily call Chemist directly instead of using service | ||
| // control client library | ||
| Call(*request, response, | ||
| [this, quota_request_copy, response, |
There was a problem hiding this comment.
just pass in check_on_done as 3rd argument.
| if (status.ok()) { | ||
| utils::Status status = Proto::ConvertAllocateQuotaResponse( | ||
| *response, service_control_proto_.service_name()); | ||
| on_done(utils::Status::OK); |
There was a problem hiding this comment.
Thank you for finding the issue
|
|
||
| auto check_on_done = [this, response, on_done, trace_span]( | ||
| const ::google::protobuf::util::Status& status) { | ||
| if (status.ok()) { |
There was a problem hiding this comment.
Removed unused captures
| const std::string& url = typeid(RequestType) == typeid(CheckRequest) | ||
| ? url_.check_url() | ||
| : url_.report_url(); | ||
| const std::string& url = |
There was a problem hiding this comment.
Consider using a separate template function get_url() for this code
There was a problem hiding this comment.
Created
template
const std::string& Aggregated::GetApiReqeustUrl(const RequestType& request)
| http_request->set_timeout_ms(config.quota_timeout_ms()); | ||
| } | ||
| } else { | ||
| if (config.report_timeout_ms() > 0) { |
There was a problem hiding this comment.
Consider move all these time_out code into a separate function
There was a problem hiding this comment.
Created
template
void Aggregated::SetHttpRequestTimeout(
const RequestType& request, std::unique_ptr& http_request);
| info->labels["servicecontrol.googleapis.com/caller_ip"] = | ||
| request_->GetClientIP(); | ||
| info->labels["servicecontrol.googleapis.com/referer"] = this->http_referer_; | ||
| info->labels["servicecontrol.googleapis.com/user"] = "integration_test_user"; |
There was a problem hiding this comment.
I don't see info->labels get used.
There was a problem hiding this comment.
This function was moved to Proto::FillAllocateQuotaRequest
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
6 participants
@qiwzhang @chowchow316
Thanks.